Variety of fixes in response to feedback on #780 #796

robscott · 2021-08-20T19:11:05Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
There's been a lot of great feedback in #780, this makes some of the easier fixes.

Does this PR introduce a user-facing change?:

* listener.routes has been renamed to listener.allowedRoutes
* The "NoSuchGatewayClass" has been removed after it was deprecated in v1alpha1
* "*" is no longer a valid hostname. Instead, leaving hostname unspecified is interpreted as "*".

k8s-ci-robot · 2021-08-20T19:11:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

robscott · 2021-08-20T23:45:00Z

apis/v1alpha2/gateway_types.go

-// Implementations should add the `gateway-exists-finalizer.gateway.networking.k8s.io`
-// finalizer on the associated GatewayClass whenever Gateway(s) is running.
-// This ensures that a GatewayClass associated with a Gateway(s) is not
-// deleted while in use.


Moved to GatewayClass spec

youngnick

Two small suggestions, but aside from that, LGTM.

apis/v1alpha2/gateway_types.go

apis/v1alpha2/gatewayclass_types.go

apis/v1alpha2/gateway_types.go

hbagdi · 2021-08-23T16:59:30Z

apis/v1alpha2/gateway_types.go

-	// ignored otherwise.
+	// TLS is the TLS configuration for the Listener. This field is required if
+	// the Protocol field is "HTTPS" or "TLS". It MUST be ignored when the
+	// Protocol field is "HTTP", "TCP", or "UDP".


I'd prefer we error out. Such silent acceptance result in confusion during debugging.
We can add a check in the validation webhook for this purpose.

Feels like this is a good case for a Condition of some sort, but I'm not sure which one.

hbagdi · 2021-08-23T17:00:20Z

apis/v1alpha2/gateway_types.go

-	// only one rule may ultimately receive the request. Matching precedence
-	// MUST be determined in order of the following criteria:
+	// Although a client request may match multiple route rules, only one rule
+	// may ultimately receive the request. Matching precedence MUST be


MUST rather than MAY.

Suggested change

// may ultimately receive the request. Matching precedence MUST be

// WILL ultimately receive the request. Matching precedence MUST be

I think one of the things we're struggling with here is what the intended audience is. I think we may need to start differentiating implementation guidance (words like MUST) with what users should expect (words like will). Maybe implementation guidance belongs somewhere other than spec?

I've struggled with that distinction and I brought it before as well.
I think most of the documentation is geared towards implementation guidance and we should stick to that. The larger audience will require much more context and the implementation authors can take care of that.

Was talking with @bowei about this recently, maybe there are some guidelines we can be applying when we're writing our godocs here?

apis/v1alpha2/gateway_types.go

apis/v1alpha2/httproute_types.go

apis/v1alpha2/shared_types.go

apis/v1alpha2/tcproute_types.go

youngnick · 2021-08-24T03:26:21Z

I think I've only really got one thing that needs addressing here, but there are a few followup things, especially around Conditions and status (which is probably due for a recheck and overhaul). I don't think we should block at least the RC on status though, and I feel like a status review is possibly bigger than we should bite off bevore v1alpha2.

ccfishk · 2021-08-24T20:37:46Z

apis/v1alpha2/gatewayclass_types.go

@@ -47,12 +60,19 @@ type GatewayClass struct {
 	Status GatewayClassStatus `json:"status,omitempty"`
 }

+const (
+	// GatewayClassFinalizerGatewaysExist MUST be added as a finalizer to the


literally "must" is already "commanded or requested", strong.
dont think the uppercase is really needed since the importance is delivered by the verb.

Suggested change

// GatewayClassFinalizerGatewaysExist MUST be added as a finalizer to the

// GatewayClassFinalizerGatewaysExist must be added as a finalizer to the

I'm not really sure how to distinguish between MUST and must for our godocs, but I think historically we've been using all caps to emphasize/match RFC terminology.

https://www.rfc-editor.org/rfc/rfc2119

apis/v1alpha2/gatewayclass_types.go

ccfishk · 2021-08-24T20:41:52Z

apis/v1alpha2/gatewayclass_types.go

+	// GatewayClassFinalizerGatewaysExist MUST be added as a finalizer to the
+	// GatewayClass whenever there are provisioned Gateways using a
+	// GatewayClass.
+	GatewayClassFinalizerGatewaysExist = "gateway-exists-finalizer.gateway.networking.k8s.io"


After reading your comments,

Suggested change

GatewayClassFinalizerGatewaysExist = "gateway-exists-finalizer.gateway.networking.k8s.io"

ProvisionedGatewayClassFinalizer = "provisioned-gateway-finalizer.gateway.networking.k8s.io"

is more aligned to what you mean ?

This probably could have a better name, but in this case I'm not actually changing the name, just moving it from gateway_types to gatewayclass_types. The idea is for it to indicate that at least one Gateway is using the GatewayClass and therefore the GatewayClass should not be deleted.

ccfishk · 2021-08-24T20:45:20Z

apis/v1alpha2/httproute_types.go

+	//
+	// * A Listener with `test.example.com` as the hostname matches HTTPRoutes
+	//   that have either not specified any hostnames, or have specified at
+	//   least one of `test.example.com` or `*.example.com`.


Suggested change

// least one of `test.example.com` or `*.example.com`.

// least one of `test.example.com` or `*.example.com` or `*`.

This is actually something that came out of API review, I can't remember which of the reviewers suggested it, but the recommendation was to remove "" as a possible value. An empty or unspecified value here would continue to be interpreted at "", but there was no point in differentiating between empty and "*" since they were interpreted the same way. Let me add that to the PR description.

apis/v1alpha2/httproute_types.go

ccfishk · 2021-08-24T20:50:53Z

apis/v1alpha2/httproute_types.go

+	// ignored. For example, if a Listener specified `*.example.com`, and the
+	// HTTPRoute specified `test.example.com` and `test.example.net`,
+	// `test.example.net` must not be considered for a match.
+	//


Do you mean as following ?
// If both the Listener and HTTPRoute have specified hostnames, only exact match
// will be considered.
// For example, if a Listener specified test.example.com, and the
// HTTPRoute specified test.example.com and test.example.net,
// test.example.net is not be considered for a match.

This is trying to describe how wildcard hostname matching should work with hostnames specified on HTTPRoutes. I don't think removing the wildcard from the example would help here, but I may be misunderstanding what you're suggesting.

ccfishk · 2021-08-24T20:54:24Z

apis/v1alpha2/httproute_types.go

-	// must get a 503 instead.
+	// sent.
+
+	// If unspecified or invalid (refers to a non-existent resource or a Service


is there any k8s Service (Service with no endpoints) that does not have endpoints ?
maybe "resource" is more accurate here ?

I think what we're trying to communicate here is a Service that does not have any endpoints, likely because there are no Pods running that have been selected by that service.

apis/v1alpha2/httproute_types.go

ccfishk · 2021-08-24T20:56:58Z

apis/v1alpha2/tcproute_types.go

-	// status for this route should be updated with a condition that describes
-	// the error more specifically.
+	// "match" behavior. For example, resource "mytcproutematcher" in group
+	// "networking.example.net". If the referent cannot be found, the rule MUST


Suggested change

// "networking.example.net". If the referent cannot be found, the rule MUST

// "networking.example.net". If the referent cannot be found, the rule must

I think there's probably room for a broader discussion around the usability of these all caps RFC terms, but so far the guidance from other reviewers has been to move more into this capitalization style. We can add to the agenda for next community meeting and/or create an issue to discuss if that's helpful. I don't have a very strong opinion here other than trying to be consistent with what we've done elsewhere.

This is definitely a thing we've copied from RFCs, where the increased emphasis is very purposeful - it's a guide for people implementing the spec that allows the reader to easily find the things they MUST do when reading what is often a long and dry specification. That's why I'm supportive of the all-caps.

howardjohn · 2021-08-21T19:04:07Z

apis/v1alpha2/gateway_types.go

-	// ignored otherwise.
+	// TLS is the TLS configuration for the Listener. This field is required if
+	// the Protocol field is "HTTPS" or "TLS". It MUST be ignored when the
+	// Protocol field is "HTTP", "TCP", or "UDP".


should ignore be actually be illegal? enforced by validation webhook?

Same with hostname for tcproute?

I agree with that interpretation. For now I've just stated that this is invalid, but also created #827 to track this.

apis/v1alpha2/gatewayclass_types.go

robscott · 2021-08-25T00:36:22Z

Thanks to everyone for the great feedback on this one! I think I've responded to everything now, PTAL. Once we get this in, we should be able to move forward with v1alpha2-rc1.

robscott · 2021-08-25T00:45:18Z

Looks like a deploy/netlify flake?

/retest

youngnick

/lgtm

I think that @ccfishk raises some fair questions to discuss, particularly about the use of all-caps for MUST, MAY etc, but I also think that's a larger discussion that would be better suited for its own issue/PR.

Signed-off-by: Nick Young <ynick@vmware.com>

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 20, 2021

k8s-ci-robot requested review from hbagdi and thockin August 20, 2021 19:11

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2021

robscott mentioned this pull request Aug 20, 2021

SIG-NETWORK v1alpha2 Review #780

Closed

robscott force-pushed the 780-small-fixes branch from 47516aa to cadb97c Compare August 20, 2021 23:41

robscott commented Aug 20, 2021

View reviewed changes

robscott force-pushed the 780-small-fixes branch 2 times, most recently from fe98e8b to 26f3a97 Compare August 21, 2021 00:09

youngnick reviewed Aug 23, 2021

View reviewed changes

apis/v1alpha2/gateway_types.go Show resolved Hide resolved

apis/v1alpha2/gatewayclass_types.go Show resolved Hide resolved

hbagdi reviewed Aug 23, 2021

View reviewed changes

robscott force-pushed the 780-small-fixes branch from 26f3a97 to 5e943f9 Compare August 23, 2021 21:54

ccfishk reviewed Aug 24, 2021

View reviewed changes

apis/v1alpha2/gatewayclass_types.go Show resolved Hide resolved

ccfishk reviewed Aug 24, 2021

View reviewed changes

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

ccfishk reviewed Aug 24, 2021

View reviewed changes

apis/v1alpha2/httproute_types.go Show resolved Hide resolved

ccfishk reviewed Aug 24, 2021

View reviewed changes

howardjohn reviewed Aug 24, 2021

View reviewed changes

Variety of fixes in response to feedback on kubernetes-sigs#780

ffd6005

robscott force-pushed the 780-small-fixes branch from 5e943f9 to f89c716 Compare August 25, 2021 00:33

robscott force-pushed the 780-small-fixes branch from f89c716 to 24e1570 Compare August 25, 2021 00:41

Updates based on latest PR feedback

0fbc16b

robscott force-pushed the 780-small-fixes branch from 24e1570 to 0fbc16b Compare August 25, 2021 00:46

youngnick reviewed Aug 25, 2021

View reviewed changes

k8s-ci-robot assigned youngnick Aug 25, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2021

k8s-ci-robot merged commit 8343da8 into kubernetes-sigs:master Aug 25, 2021

robscott mentioned this pull request Aug 25, 2021

Update CHANGELOG.md for v0.4.0-rc1 #791

Merged

robscott added a commit to robscott/gateway-api that referenced this pull request Aug 25, 2021

Changes from kubernetes-sigs#796

d4fb63d

youngnick pushed a commit to youngnick/gateway-api that referenced this pull request Aug 26, 2021

Add in changes from kubernetes-sigs#796

0def934

Signed-off-by: Nick Young <ynick@vmware.com>

robscott deleted the 780-small-fixes branch January 8, 2022 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Variety of fixes in response to feedback on #780 #796

Variety of fixes in response to feedback on #780 #796

robscott commented Aug 20, 2021 •

edited

Loading

k8s-ci-robot commented Aug 20, 2021

robscott Aug 20, 2021 •

edited

Loading

youngnick left a comment

hbagdi Aug 23, 2021

youngnick Aug 24, 2021

hbagdi Aug 23, 2021

robscott Aug 23, 2021

hbagdi Aug 23, 2021

robscott Aug 24, 2021

youngnick commented Aug 24, 2021

ccfishk Aug 24, 2021 •

edited

Loading

robscott Aug 24, 2021

jpeach Aug 25, 2021

ccfishk Aug 24, 2021

robscott Aug 24, 2021

ccfishk Aug 24, 2021

robscott Aug 24, 2021

ccfishk Aug 24, 2021

robscott Aug 24, 2021

ccfishk Aug 24, 2021 •

edited

Loading

robscott Aug 24, 2021

ccfishk Aug 24, 2021

robscott Aug 25, 2021

youngnick Aug 25, 2021

howardjohn Aug 21, 2021

robscott Aug 25, 2021

robscott commented Aug 25, 2021

robscott commented Aug 25, 2021

youngnick left a comment

	// may ultimately receive the request. Matching precedence MUST be
	// WILL ultimately receive the request. Matching precedence MUST be

	// GatewayClassFinalizerGatewaysExist MUST be added as a finalizer to the
	// GatewayClassFinalizerGatewaysExist must be added as a finalizer to the

	GatewayClassFinalizerGatewaysExist = "gateway-exists-finalizer.gateway.networking.k8s.io"
	ProvisionedGatewayClassFinalizer = "provisioned-gateway-finalizer.gateway.networking.k8s.io"

	// least one of `test.example.com` or `*.example.com`.
	// least one of `test.example.com` or `.example.com` or ``.

	// "networking.example.net". If the referent cannot be found, the rule MUST
	// "networking.example.net". If the referent cannot be found, the rule must

Variety of fixes in response to feedback on #780 #796

Variety of fixes in response to feedback on #780 #796

Conversation

robscott commented Aug 20, 2021 • edited Loading

k8s-ci-robot commented Aug 20, 2021

robscott Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick commented Aug 24, 2021

ccfishk Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccfishk Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Aug 25, 2021

robscott commented Aug 25, 2021

youngnick left a comment

Choose a reason for hiding this comment

robscott commented Aug 20, 2021 •

edited

Loading

robscott Aug 20, 2021 •

edited

Loading

ccfishk Aug 24, 2021 •

edited

Loading

ccfishk Aug 24, 2021 •

edited

Loading